[java] Add linux-x64 implementation of in process Copilot CLI - #2301
[java] Add linux-x64 implementation of in process Copilot CLI#2301edburns wants to merge 1 commit into
Conversation
Squashed from PR #2295 (branch edburns/…-review-02). Includes Java multi-module Maven restructure, copilot-native submodule for bundling the Rust CLI runtime, codegen updates, and related workflow changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 90cbda40-cda3-4ecd-b381-9f9ba0573d0a
Cross-SDK Consistency Review ✅This PR adds the in-process FFI runtime connection to the Java SDK, bringing it to parity with all other SDK implementations. Feature parity check
API naming consistencyThe Java implementation follows the expected language idioms:
ConclusionNo cross-SDK consistency issues found. This PR completes the in-process FFI feature across all six SDK languages.
|
There was a problem hiding this comment.
@edburns here are some comments from my review agent, hope these make sense. Happy to take another human look afterwards!
Requesting changes. The overall transport architecture broadly aligns with the other SDKs, and the large file count is mostly explainable: 1,522 of 1,596 files are byte-identical moves into java/sdk/. The module split is reasonable, but the published dependency graph, native ABI/lifecycle, and release validation still have blocking issues.
GitHub cannot attach inline review comments to unchanged files, so these relocation omissions are called out here:
.github/workflows/java-publish-maven.yml:204,207still referencesjava/jbang-example.java, so release preparation will fail after the move tojava/sdk/jbang-example.java.scripts/docs-validation/validate.ts:388-394searches the parent POM for artifactcopilot-sdk-java; it now falls back to1.0.0-SNAPSHOTinstead of validating the reactor's1.0.11-preview.0-SNAPSHOTartifact..github/actions/java-test-report/action.yml:7,11,15still searchesjava/target/**; current CI logs report that no test reports were found even though results are underjava/sdk/target/**..github/workflows/java-smoke-test.yml:66,139still points to the pre-move prompt path.
Please address the inline findings and these unchanged-file omissions before merging.
|
|
||
| <readonly-copilot-sdk-ref-impl-version-from-lastmerge-file-updated-by-reference-impl-sync>^1.0.79-6</readonly-copilot-sdk-ref-impl-version-from-lastmerge-file-updated-by-reference-impl-sync> | ||
| <!-- The parent POM is not published to Maven Central. --> | ||
| <maven.deploy.skip>true</maven.deploy.skip> |
There was a problem hiding this comment.
Blocker: Both published child POMs inherit from copilot-sdk-java-parent, but this prevents that parent from being deployed and there is no flattening step. Maven consumers resolving copilot-sdk-java or copilot-sdk-java-runtime will then fail to resolve their parent POM. Please publish the parent or deploy flattened child POMs.
| --> | ||
| <readonly-copilot-sdk-ref-impl-version-from-lastmerge-file-updated-by-reference-impl-sync>^1.0.79-9</readonly-copilot-sdk-ref-impl-version-from-lastmerge-file-updated-by-reference-impl-sync> | ||
|
|
||
| <readonly-copilot-sdk-ref-impl-version-from-lastmerge-file-updated-by-reference-impl-sync>^1.0.79-6</readonly-copilot-sdk-ref-impl-version-from-lastmerge-file-updated-by-reference-impl-sync> |
There was a problem hiding this comment.
High: This pin is being downgraded from ^1.0.79-9 to ^1.0.79-6, and the regenerated public API consequently loses current types and fields such as SandboxConfigAuth, factory-agent options, SubagentCompletedEvent.cancelled, and paged listRuns. This is unrelated to the FFI work and rolls main backwards. Please restore the current pin and regenerate.
| <!-- Native runtime for linux-x64 (~20-26 MB) --> | ||
| <dependency> | ||
| <groupId>com.github</groupId> | ||
| <artifactId>copilot-sdk-java-runtime</artifactId> |
There was a problem hiding this comment.
Blocker: These two artifacts are not sufficient for in-process mode. The SDK declares JNA as optional, and the runtime artifact does not depend on JNA, so Maven will not bring it transitively. Users following this snippet will hit NoClassDefFoundError when selecting the in-process transport. Please make the runtime artifact bring JNA transitively or document an explicit third dependency.
| try { | ||
| return lib.copilot_runtime_connection_close(connectionId) != 0; | ||
| } finally { | ||
| trackedCallbacks.remove(connectionId); |
There was a problem hiding this comment.
High: Removing the wrapper in finally releases its only GC root even when native connection_close fails or throws. Native code may still retain and invoke that function pointer, which can crash the JVM after JNA collects the callback. Keep the wrapper rooted through callback draining and host shutdown, and only release it when native ownership has definitely ended.
| </plugin> | ||
| <!-- | ||
| Required by Maven Central: sources and javadoc artifacts. This | ||
| module has no Java sources, so both produce empty archives. |
There was a problem hiding this comment.
High: This claim does not match the release output. Building this module with -Prelease emits the main and sources JARs but logs No Javadoc in project. Archive not created; Maven Central requires a javadoc JAR for every non-POM artifact. Please attach an explicit placeholder javadoc JAR for this source-less module.
| * @param len | ||
| * byte length of the buffer pointed to by {@code data} | ||
| */ | ||
| void invoke(Pointer userData, Pointer data, int len); |
There was a problem hiding this comment.
Medium: The C ABI declares this length as size_t, but Java int is always 32-bit and therefore mismatches the 64-bit ABI. The other SDKs use c_size_t, size_t, nuint, or usize. Please use a JNA IntegerType sized with Native.SIZE_T_SIZE here and for every native length parameter.
| this.receiveStream = Objects.requireNonNull(receiveStream, "receiveStream must not be null"); | ||
| this.sendStream = new FfiOutputStream(this.nativeBinding, this.connectionId, this.closing, this.operationLock); | ||
| this.libraryPath = libraryPath; | ||
| Native.setCallbackExceptionHandler((Callback callback, Throwable throwable) -> LOG.log(Level.WARNING, |
There was a problem hiding this comment.
Medium: This replaces JNA's process-wide callback exception handler for the entire hosting application and never restores it, changing the behavior of unrelated JNA callbacks. The local outbound callback already catches and logs failures, so please avoid this global mutation or preserve and restore the prior handler.
| name: "Java SDK InProcess Tests" | ||
| if: github.event.repository.fork == false | ||
| runs-on: ubuntu-latest | ||
| continue-on-error: true |
There was a problem hiding this comment.
Medium: Making the only real in-process test job continue-on-error means FFI regressions can never block this PR or later merges. Since this change introduces the transport, this job should be required once it is added.
| You can run the SDK without setting up a full Java project, by using [JBang](https://www.jbang.dev/). | ||
|
|
||
| See the full source of [`jbang-example.java`](jbang-example.java) for a complete example with more features like session idle handling and usage info events. | ||
| See the full source of [`jbang-example.java`](sdk/jbang-example.java) for a complete example with more features like session idle handling and usage info events. |
There was a problem hiding this comment.
Medium: This updates the source link after the move, but the runnable JBang URL immediately below still points to java/jbang-example.java. The release workflow also retains that old path. Please update both remaining references to java/sdk/jbang-example.java.
| - `.overridesBuiltInTool(boolean)` — shadow built-in tools | ||
|
|
||
| For design context and decision rationale, see [ADR-006](docs/adr/adr-006-tool-definition-inline.md). | ||
| For design context and decision rationale, see [ADR-006](sdk/docs/adr/adr-006-tool-definition-inline.md). |
There was a problem hiding this comment.
Medium: The ADRs remain under java/docs/adr/, so sdk/docs/adr/... does not exist. This link should remain docs/adr/adr-006-tool-definition-inline.md; the ADR-004 link later in the README needs the same correction.
roji
left a comment
There was a problem hiding this comment.
Here are a few more comments.
Another thing I noticed is that while all other language SDKs automatically download the correct platform package with the native binary, the current approach in this PR requires users to manually take a dependency on e.g. the linux-x64 package, in addition to the platform-agnostic SDK package.
I don't know anything about how this kind of thing works with Java/Maven; is it impossible/not "the right way" to offer something that does this automatically (as all the other SDKs do)? Or maybe you're planning to look at that separately in a later PR (obviously completely fine too). Just raising the question.
| contents: read | ||
|
|
||
| jobs: | ||
| java-sdk-inprocess: |
There was a problem hiding this comment.
Does it make sense to integrate this in the regular job as a matrix ([default, inprocess]) rather than having a completely separate leg for it?
| Additional classifiers are added in a later phase, each with its own | ||
| fetch execution and maven-jar-plugin execution. | ||
| --> | ||
| <copilot.native.classifier>linux-x64</copilot.native.classifier> |
There was a problem hiding this comment.
Does this mean we don't support any other platform (linux-arm64, Windows/Mac)? Of course totally fine if you want to do this incrementally in separate PRs, just pointing it out.
| String classifier = PlatformDetector.detectClassifier(); | ||
| String version = readVersion(loader); | ||
| Path cacheBase = defaultCacheBase(); | ||
| return resolve(null, findRuntimeOnPath(), cacheBase, loader, classifier, version); |
There was a problem hiding this comment.
Copilot review comment:
Could we avoid automatically falling back to an arbitrary copilot found on PATH for in-process mode? The embedded native ABI must stay compatible with the CLI/runtime pair, while a PATH installation can be any version and may produce difficult-to-diagnose ABI skew. The other SDKs use bundled/pinned assets or an explicit COPILOT_CLI_PATH; requiring one of those here would keep Java aligned and make runtime selection deterministic.
Supercedes #2295 .
This PR is the roll up of the agentic work done in the subtasks of #2166 . At each step of those subtasks, the CI was clean and all reviews were applied as appropriate.
PR 2295 — Reviewer's guide: In-process FFI runtime for the Java SDK
TL;DR
This PR does for the Java SDK what #1901 did for .NET and #1915 did for Rust: it adds an in-process connection mode that loads the Copilot runtime (
runtime.nodecdylib) as a native library via JNA, eliminating the need for a separate CLI child process. Currently scoped to linux-x64 only; the entire in-process API surface is marked@CopilotExperimental.The PR also restructures the Java Maven project from a single module into a multi-module reactor to support publishing the native runtime binaries as separate classifier JARs alongside the existing SDK JAR.
What's in the native binary, where does it come from, and how is it loaded?
The binary:
runtime.nodeDespite the
.nodeextension (a napi-rs naming convention),runtime.nodeis an ordinary platform-specific shared library (.soon Linux). It is a Rustcdylibproduced by thesrc/runtimecrate ingithub/copilot-agent-runtime. It exposes two front doors:extern "C"lifecycle/transport entry points callable by any language via FFI without Node.js.The 5 C ABI entry points are:
copilot_runtime_host_startcopilot_runtime_host_shutdowncopilot_runtime_connection_openon_outboundcallback for runtime→SDK data delivery.copilot_runtime_connection_writecopilot_runtime_connection_closeAll JSON-RPC methods travel as data through this fixed 5-function transport; the export surface never changes as the method set grows.
Where it comes from (build-time)
The
copilot-nativeMaven module's build fetches the binary from npm duringgenerate-resources:fetch-native.mjsreads the pinned version and SHA-512 integrity hash for@github/copilot-linux-x64fromnodejs/package-lock.json.npm packto download the exact tarball, verifies it against the integrity hash.runtime.nodeand thecopilotCLI executable into a staging directory.maven-jar-pluginpackages them into a classifier JAR (copilot-sdk-java-runtime-<version>-linux-x64.jar) with the layoutnative/linux-x64/runtime.node.How it's loaded (runtime)
PlatformDetector(303 lines) determines the classifier usingos.name,os.arch, and on Linux, ELF PT_INTERP parsing to distinguish glibc vs musl — no subprocesses, no heuristics.NativeRuntimeLoader(466 lines) resolves the binary in this order:COPILOT_CLI_PATHenv var → checks forruntime.nodealongside the CLI.native/<classifier>/runtime.node→ extracts atomically to~/.copilot/runtime-cache/<version>/<classifier>/runtime.node.runtime.nodealongside the bundledcopilotexecutable.JnaNativeBinding(253 lines) loads the library by absolute path via JNA and maps each C ABI export. Enforces a one-library-per-process invariant (library handle isstatic, never unloaded). Duplicate loads from the same path are silently accepted; different paths are rejected.FfiRuntimeHost(349 lines) orchestrates the lifecycle: starts the host, opens a connection, bridges the bidirectional JSON-RPC transport. Theon_outboundcallback (invoked by native threads) feeds received data into aQueueInputStreamthat the SDK's existingJsonRpcClientreads from.Structural changes
Multi-module Maven reactor
The single-module
java/pom.xmlis now a parent POM (pompackaging) with two submodules:java/pom.xmlcopilot-sdk-java-parentmaven.deploy.skip=true). Holds thereleaseprofile (GPG signing) inherited by all submodules.java/sdk/copilot-sdk-javajava/src/→java/sdk/src/.java/copilot-native/copilot-sdk-java-runtimelinux-x64only, ~20-26 MB).Consumer dependency declaration
Consumer usage
New public API surface (all
@CopilotExperimental)RuntimeConnection(sealed class)forStdio(),forTcp(),forUri(String),forInProcess().StdioRuntimeConnectionTcpRuntimeConnectionUriRuntimeConnectionInProcessRuntimeConnectionCopilotClientOptions.setConnection()/getConnection()The
RuntimeConnectionAPI replaces the previous pattern of settingcliUrl,cliPath,useStdio,port, andtcpConnectionTokenindividually. When aRuntimeConnectionis set, it takes precedence; conflicting legacy options causeIllegalArgumentException.New internal packages
com.github.copilot.ffi(9 classes, ~1,752 lines)FfiRuntimeHostJnaNativeBindingNativeBindingNativeRuntimeLoaderruntime.node: env var → classpath → cache. Atomic extraction with file locking.PlatformDetectorQueueInputStreamFfiOutputStreamconnection_write.OutboundCallbackon_outbound.ReaderThreadFactoryTests for FFI (6 files, ~2,054 lines)
FfiRuntimeHostTestJnaNativeBindingTestNativeRuntimeLoaderTestPlatformDetectorTestQueueInputStreamTestInProcessTransportITCI/workflow changes
java-sdk-inprocessinjava-sdk-tests.yml: runsmvn clean verify -Pinprocesson ubuntu-latest (linux-x64). Usescontinue-on-error: truewhile experimental.java/target/→java/sdk/target/for surefire/failsafe reports and coverage data.-pl sdkto restrict to the SDK module (the native module requires JDK 25 build tools).java/sdk/module layout.✅ Note that the existing java publishing jobs will continue to work as currently written.
Key design decisions (from ADR-007)
JNA over Panama FFM: JNA supports the Java 17 baseline with zero consumer configuration. Panama FFM requires Java 22+ and
--enable-native-accessflags. Performance difference is irrelevant (JSON-RPC I/O dominates).Per-platform classifier JARs over monolithic JAR: A monolithic JAR with all 6 common platforms would be ~132 MB. Classifier JARs let consumers pull only their target platform (~20-26 MB each). An uber-JAR can be assembled via
maven-assembly-pluginif needed.Library-never-unloads pattern: The loaded native library is held in a
staticfield and never released. Native worker threads outlive any individualFfiRuntimeHostinstance; unloading would crash.One library per process: Enforced by a process-wide guard, consistent with Rust, .NET, Go, and Python SDK implementations.
Diff statistics
java/src/→java/sdk/src/)copilot-native/pom.xml(214 lines),fetch-native.mjs(114 lines)Recommended review order
java/docs/adr/adr-007-native-bundling-strategy.md— context, options considered, decision rationale.rpc/RuntimeConnection.java,rpc/InProcessRuntimeConnection.java, andrpc/CopilotClientOptions.java(thesetConnection/getConnectionmethods).NativeBinding.java→JnaNativeBinding.java→FfiRuntimeHost.java→NativeRuntimeLoader.java→PlatformDetector.java.copilot-native/pom.xmlandcopilot-native/scripts/fetch-native.mjs.java/pom.xml(parent) andjava/sdk/pom.xml(child)..github/workflows/java-sdk-tests.yml(new inprocess job, path updates).ffi/test package ande2e/InProcessTransportIT.java.Implementation details.
Implemented agentically using https://aka.ms/coreai/shepherd-task/slides .